Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Removed 'goto' from 'LineFormatter' and other code refactorings#168

Merged
AlexanderSher merged 11 commits intomicrosoft:masterfrom
AlexanderSher:master
Oct 3, 2018
Merged

Removed 'goto' from 'LineFormatter' and other code refactorings#168
AlexanderSher merged 11 commits intomicrosoft:masterfrom
AlexanderSher:master

Conversation

@AlexanderSher
Copy link
Copy Markdown
Contributor

This PR merges #162.

  • goto keywords are removed, break statements that belong to the switch flow are all straighten to avoid confusion with break statements in loops
  • Added TextEditCollectionAssertions to get better messages for test failures
  • Various clean-ups

@AlexanderSher
Copy link
Copy Markdown
Contributor Author

AlexanderSher commented Sep 28, 2018

Couple of follow-up questions:

  • In TokenKind.Power and TokenKind.Multiply cases, when actualPrev == null, default are different. Is that correct?
  • In production, we never reuse the LineFormatter, so its cache is never used. Are we going to use it in the future?

Comment thread src/Analysis/Engine/Test/LineFormatterTests.cs
Comment thread src/LanguageServer/Impl/Implementation/LineFormatter.cs
var tokenExt = new TokenExt {
Token = token,
PreceedingWhitespace = _tokenizer.PreceedingWhiteSpace,
PrecedingWhitespace = _tokenizer.PreceedingWhiteSpace,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a comment on your code, but "preceding" is misspelled in the tokenizer too and should probably be fixed at some point. (That's probably why I also misspelled it.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Public property of public class. @MikhailArkhipov , can we rename that?

Comment thread src/LanguageServer/Impl/Implementation/LineFormatter.cs Outdated
Comment thread src/Analysis/Engine/Test/FluentAssertions/TextEditCollectionAssertions.cs Outdated
Comment thread src/Analysis/Engine/Test/LineFormatterTests.cs
Comment thread src/Analysis/Engine/Test/FluentAssertions/AssertionsUtilities.cs Outdated
Comment thread src/LanguageServer/Impl/Implementation/LineFormatter.cs Outdated
@jakebailey
Copy link
Copy Markdown
Member

In TokenKind.Power and TokenKind.Multiply cases, when actualPrev == null, default are different. Is that correct?

Yes, I think that's correct. actualPrev can only ever be null when the token being examined is the first token in the file. You want to handle a case like:

*a, b, c = [1, 2, 3, 4, 5]

Where * comes first, but the same code with ** at the start wouldn't make syntactic sense, so I chose not to handle it.

In production, we never reuse the LineFormatter, so its cache is never used. Are we going to use it in the future?

My intention was to write a simple full-document formatter and potentially reuse the formatter for formatting of the same file multiple times (since tokens do not change), but I don't think either is really a good idea or practical. What it's doing right now is allowing for less-complicated logic for moving \ to the previous line's list of tokens. Otherwise, if you threw away the top-level list and just looked for tokens which had a specific line number, you'd have to add a bunch of extra logic to ensure that no \ are missed.

- Use `Range.ToString()` in tests
- Add test case for `\r\n` line break (test fails).
  - Made line 0-based, as it is in `Tokenizer` and LSP.
  - Removed `Peek` from `TokenizerWrapper` - it isn't needed, checking current token is enough.
  - Simplified the code by removing all usages of `SourceLocation` and `SourceSpan`: those types aren't required and conversion to `line/character` representation is needed only at the very end of `FormatLine` method.
  - Added `GetLineStartIndex` and `GetLineEndIndex` methods, cause `NewLineLocation` doesn't have that information.
- Fixed `GrammarFile` test. It has been taking substring of length `end.character - start.character - 1`. `Range.end` is exclusive, so length should be calculated as `end.character - start.character`
- Added couple of helper methods to the `Tokenizer`
@AlexanderSher
Copy link
Copy Markdown
Contributor Author

Where * comes first, but the same code with ** at the start wouldn't make syntactic sense, so I chose not to handle it.

So do we format code that has syntax errors?

Otherwise, if you threw away the top-level list and just looked for tokens which had a specific line number, you'd have to add a bunch of extra logic to ensure that no \ are missed.

Got it.

@jakebailey
Copy link
Copy Markdown
Member

So do we format code that has syntax errors?

The line formatter will run regardless of syntax. Unpacking was one of the pain points of Mikhail's original implementation in VS Code, and we essentially get this specific handling for free by using PrevNonIgnored instead of Prev.

@MikhailArkhipov
Copy link
Copy Markdown

Yes we do format code with errors. Other editors do it too.

@AlexanderSher AlexanderSher merged commit aaef67e into microsoft:master Oct 3, 2018
jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
Removed 'goto' from 'LineFormatter' and other code refactorings
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants